fix: properly handle external redirects #9590
Conversation
🦋 Changeset detectedLatest commit: 21d58a2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| @@ -5433,7 +5433,7 @@ describe("a router", () => { | |||
| it("preserves query and hash in redirects", async () => { | |||
| let t = setup({ routes: REDIRECT_ROUTES }); | |||
|
|
|||
| let nav1 = await t.fetch("/parent/child", { | |||
| let nav1 = await t.navigate("/parent/child", { | |||
There was a problem hiding this comment.
These didn't need to be fetches, that was just as copy/paste from a prior test
| ) { | ||
| window.location.replace(redirect.location); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Just navigate directly for external redirects instead of "starting" a new router navigation
| path === "/" ? basename : joinPaths([basename, path]); | ||
| } | ||
| // Support relative routing in internal redirects | ||
| if (!external) { |
There was a problem hiding this comment.
Don't do any post-processing of the redirect location if it's an external location
There was a problem hiding this comment.
I'm in the middle of migrating parts of a PHP website to remix, using nginx to route some urls to remix. In my very particular case, if I redirect() from Remix to a PHP page, the origin would be the same, and this check would fail 😬
Would testing for a scheme pattern (regex, or a loose check on ://) instead negatively impact perfs?
There was a problem hiding this comment.
Hm, good call. Is that something you can do in Remix today? Or does it have this same issue?
There was a problem hiding this comment.
If we go with a manual check, I think we'd need to check for both protocol-less URLs (//google.com/path) as well as maybe a loose protocol check for ://. Perf shouldn't be a concern since this isn't a particularly hot code path - O(n) where n is the # of loaders/actions being run on a given navigation
There was a problem hiding this comment.
Remix has the same issue today.
I'm not sure we want to support protocol-less urls, as far as I know, those are only meant to apply the current scheme in HTML.
MDN says relative or absolute, and RFC 9110 seems to require a : in absolute URIs.
But that's a long document and my nerdiness is failing me 😅
Also, if my http:// remix app, is behind a https:// nginx proxy, what scheme would //my-domain/page refer to? Doesn't seem that trivial from a server side perspective.
There was a problem hiding this comment.
ok yeah I figured it should since I stole the approach from there ;)
I haven't dug into the spec, but was more considering this from a browser-emulator perspective. I did some quick testing and protocol-less URLs work in both of the redirect mechanisms at play here - neither of which are truly "handled" by RR/Remix - we're just handing off a location provided by the user to a built-in browser mechanism.
Document redirects - returning a 301 with Location: //google.com works
Client-side redirects - window.location.replace('//google.com') works
So I don't think we need to implement the logic on how to handle the redirect, so much as decide when to hand off the Location provided by the user directly to the browser instead of assuming it's a path inside our app. If the browser is doing something not-technically-spec-compliant, we might be able to leave that as a browser concern?
There was a problem hiding this comment.
@machour Do you mind creating a separate proposal for "same domain hard redirects"? I'm going to merge this since we need this external redirect handling in the RRR work - but I don't want to lose sight of that potential enhancement
Properly handle external redirects from actions and loaders:
Client-side we detect and process these via
window.location.replacelike we do in Remix currently. In SSR scenarios we return the redirect location untouched.This fixes an issue discovered while testing the "Remix on React Router 6.4" work (remix-run/remix#4570)